Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completion for nushell #450

Closed
wants to merge 2 commits into from
Closed

Conversation

include-windowsh
Copy link

I have written a completion file.
And write some code to fit it.

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thanks you for your contribution :)

@@ -0,0 +1,191 @@
module pueue_completions {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this file to a utils/completions/ directory.

The src folder is reserved for rust code and things that're tightly integrated with that code.
the src/bin folder specifically should only contain entry points for binaries.

Comment on lines +106 to +114
// Clap **doesn't** support nushell
// So I have written a completion script for nushell
fn generate_nushell_completion(output_directory: &PathBuf) -> std::result::Result<PathBuf, std::io::Error>{
let path = output_directory.join("pueue.nu");
//It should be in pueue directory
fs::write(path.clone(), include_bytes!("pueue.nu"))?;
Ok(path)

}
Copy link
Owner

@Nukesor Nukesor Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not comfortable with having this in the generate command, since this creates the impression that this script will be auto-generated just like the others.

As soon as I'm going to change something on the CLI interface, this script will be outdated.

-> It would be great if you could revert all changes in the actual rust code. I think its best if that file is located somewhere next to the actual code to make sure that people understand that this won't update itself.

@@ -0,0 +1,191 @@
module pueue_completions {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a small comment, that this file is hand-written and needs to be updated whenever there're any changes to the cli interface :)?

@github-actions
Copy link

Test Results

    3 files    22 suites   2m 52s ⏱️
140 tests 140 ✔️ 0 💤 0
300 runs  300 ✔️ 0 💤 0

Results for commit 1037d84.

@include-windowsh
Copy link
Author

Sorry to reply this so late :( I was busy removing my system on btrfs so I haven't seen it.
That is true on its outdated problem.
If I need a completion for pueue I should add supports for clap. And my code style have some bad habits in fact.
All in all sorry to cause this trouble, it shouldn't be pulled :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants